Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DDW-376] Fix address validation on the Shelley QA and Staging networks #2218

Merged
merged 57 commits into from
Oct 29, 2020
Merged

[DDW-376] Fix address validation on the Shelley QA and Staging networks #2218

merged 57 commits into from
Oct 29, 2020

Conversation

DeeJayElly
Copy link
Contributor

@DeeJayElly DeeJayElly commented Oct 27, 2020 •
edited by nikolaglumac
Loading

This PR fixes Address validation on Shelley QA and Staging networks and adds Shelley QA network SMASH server URL.

Todos

  • Fix Shelley QA network address validation
  • Check Staging network address validation (@DeeJayElly)
  • Add Shelley QA SMASH server URL

Screenshots

Screen Shot 2020-10-27 at 6 16 28 PM

Screen Shot 2020-10-27 at 6 16 00 PM

Testing Checklist

Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

DeeJayElly and others added 30 commits September 28, 2020 14:46
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
98d2803
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
05c794c
@DeeJayElly
DDW-373 syncing animation on wallet dropdown - changelog
53ce9f0
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
e5140ad
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
a33231e
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
bca48b6
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
442db65
@DeeJayElly
DDW-373 syncing animation on wallet dropdown
3c443c6
@DeeJayElly
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
b007aab 
…-dropdown
@DeeJayElly
DDW-373 syncing animation on wallet dropdown - fixes
3e45919
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown
cd34e9d
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - fixes for step 3 failu…
4a4a8f3 
…re dialog
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - fixes for step 3 failu…
b4a577a 
…re dialog
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - svg fix
85c1527
@DeeJayElly
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
c550b9f 
…-dropdown
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - wallet name + jp trans…
dc89a85 
…lations
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - wallet name + jp trans…
411c5d2 
…lations
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - fixes
67bbf4e
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - fixes
09b21e0
@nikolaglumac
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
b027405 
…-dropdown
@nikolaglumac
[DDW-373] Update CHANGELOG
1f4097b
@nikolaglumac
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
da99c56 
…-dropdown
@nikolaglumac
[DDW-373] Update CHANGELOG
9a9ef29
@DeeJayElly
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
b3c787c 
…-dropdown
@DeeJayElly
Merge remote-tracking branch 'origin/chore/ddw-373-syncing-animation-…
95a5ed6 
…on-wallet-dropdown' into chore/ddw-373-syncing-animation-on-wallet-dropdown

# Conflicts:
#	CHANGELOG.md
@DeeJayElly
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
5398c9f 
…-dropdown
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - fixes
bc791b2
@DeeJayElly
Merge remote-tracking branch 'origin/chore/ddw-373-syncing-animation-…
edf9aa5 
…on-wallet-dropdown' into chore/ddw-373-syncing-animation-on-wallet-dropdown
@DeeJayElly
DDW-373 Syncing animation on wallet dropdown - fixes for spinner
bef36db
@DeeJayElly
Merge branch 'develop' into chore/ddw-373-syncing-animation-on-wallet…
25a6ee8 
…-dropdown
@DeeJayElly
Merge remote-tracking branch 'origin/fix/ddw-376-fix-address-validati…
7af9350 
…on-on-shelley-qa-network' into fix/ddw-376-fix-address-validation-on-shelley-qa-network
miorsufianiohk
miorsufianiohk approved these changes Oct 28, 2020
View reviewed changes
Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Good work @DeeJayElly 👍

@gabriela-ponce
Copy link

@DeeJayElly this is working well. I was wondering if the same issue with Staging is going to be addressed on this PR too.
@alan-mcnicholas mentioned it happens there as well.

@nikolaglumac
Copy link
Contributor

@DeeJayElly this is working well. I was wondering if the same issue with Staging is going to be addressed on this PR too.
@alan-mcnicholas mentioned it happens there as well.

@DeeJayElly please check this 🙏

@nikolaglumac nikolaglumac changed the title [DDW-376] Fix address validation on shelley qa network [DDW-376] Fix address validation on the Shelley QA network Oct 29, 2020
@nikolaglumac
Copy link
Contributor

@DeeJayElly I have added one more TODO for you:
Screenshot 2020-10-29 at 10 46 34

cc @alan-mcnicholas

@DeeJayElly
Copy link
Contributor Author

@DeeJayElly I have added one more TODO for you:
Screenshot 2020-10-29 at 10 46 34

cc @alan-mcnicholas

Fix is added for network tag

@DeeJayElly DeeJayElly removed the WIP label Oct 29, 2020
@nikolaglumac nikolaglumac changed the title [DDW-376] Fix address validation on the Shelley QA network [DDW-376] Fix address validation on the Shelley QA and Staging networks Oct 29, 2020
@alan-mcnicholas
Copy link

Address validation is now working on staging, great work @DeeJayElly ! :)

@nikolaglumac
Copy link
Contributor

Address validation is now working on staging, great work @DeeJayElly ! :)

Thanks @alan-mcnicholas! Did you also make sure non-staging addresses are rejected? E.g. the mainnet ones?

cc @gnpf @ManusMcCole @mioriohk

@alan-mcnicholas
Copy link

@nikolaglumac I checked that address discrimination is working between staging\mainnet and testnet.
Valid addresses for both are accepted and invalid ones are rejected as expected due to incompatible networkMagic.

@nikolaglumac
Copy link
Contributor

@nikolaglumac I checked that address discrimination is working between staging\mainnet and testnet.
Valid addresses for both are accepted and invalid ones are rejected as expected due to incompatible networkMagic.

Great! Thanks for checking that @alan-mcnicholas ❤️
Can you please approve the PR?

alan-mcnicholas
alan-mcnicholas approved these changes Oct 29, 2020
View reviewed changes
Copy link

@alan-mcnicholas alan-mcnicholas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, great work!

nikolaglumac
nikolaglumac approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great @DeeJayElly 👍

@nikolaglumac nikolaglumac merged commit 268ec01 into develop Oct 29, 2020
@iohk-bors iohk-bors bot deleted the fix/ddw-376-fix-address-validation-on-shelley-qa-network branch October 29, 2020 14:10
@nikolaglumac nikolaglumac added release-2.4.1-FC1 Daedalus Flight and removed ⏳release-vNext labels Nov 5, 2020
@nikolaglumac nikolaglumac mentioned this pull request Nov 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alan-mcnicholas alan-mcnicholas alan-mcnicholas approved these changes

@nikolaglumac nikolaglumac nikolaglumac approved these changes

@gabriela-ponce gabriela-ponce Awaiting requested review from gabriela-ponce

@miorsufianiohk miorsufianiohk Awaiting requested review from miorsufianiohk

Assignees

@DeeJayElly DeeJayElly

Labels
bug release-2.4.1-FC1 Daedalus Flight
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DeeJayElly @gabriela-ponce @nikolaglumac @alan-mcnicholas @miorsufianiohk